Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CI] Upgrade to abseil 20240116.1 (CMake only) #2599

Merged
merged 12 commits into from
Mar 28, 2024

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Mar 21, 2024

Upgrade to abseil 20240116.1

Changes

  • Upgrade to abseil 20240116.1, but only for the CMake build.
  • The bazel build is unchanged by this PR.

See related issue #2619.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@marcalff marcalff requested a review from a team March 21, 2024 16:08
@lalitb
Copy link
Member

lalitb commented Mar 21, 2024

Bazel build is failing with:

external/com_google_absl/absl/base/policy_checks.h:79:2: error: #error "C++ versions less than C++14 are not supported."

@marcalff
Copy link
Member Author

Bazel build is failing with:

external/com_google_absl/absl/base/policy_checks.h:79:2: error: #error "C++ versions less than C++14 are not supported."

Yes, saw this.

Bazel is supposed to use --cxxopt=-std=c++14, I don't understand why it still fails.

@keith
Copy link
Contributor

keith commented Mar 21, 2024

I suggest #2600 to fix that C++14 issue

@keith
Copy link
Contributor

keith commented Mar 21, 2024

I assume the core issue is that https://github.com/open-telemetry/opentelemetry-cpp/pull/2600/files#diff-46043b297959e098526162a2aa686e9f27bf41881f5d0ed6cdab21e4cb2acd13L73 this line only has --cxxopt and not --host_cxxopt

@keith
Copy link
Contributor

keith commented Mar 21, 2024

with my change + this change I do see what I think are real compiler errors tho (on macOS), but we'll see what you see on CI:

bazel-out/darwin_arm64-fastbuild/bin/api/_virtual_includes/api/opentelemetry/nostd/./internal/absl/types/../utility/utility.h:344:33: error: reference to 'decay_t' is ambiguous
          std::tuple_size<absl::decay_t<Tuple>>::value>{});
                                ^
external/com_google_absl/absl/meta/type_traits.h:304:1: note: candidate found by name lookup is 'absl::lts_20240116::decay_t'
using decay_t = typename std::decay<T>::type;
^
bazel-out/darwin_arm64-fastbuild/bin/api/_virtual_includes/api/opentelemetry/nostd/./internal/absl/types/../utility/../base/internal/../../meta/type_traits.h:627:1: note: candidate found by name lookup is 'absl::otel_v1::decay_t'
using decay_t = typename std::decay<T>::type;
^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.

@marcalff
Copy link
Member Author

Thanks @keith for the help.

Any insight on what is going on here is most welcome, personally I can not make any sense of the error reported during the build.

@keith
Copy link
Contributor

keith commented Mar 21, 2024

I noticed that as well. I discovered that passing --@//api:with_abseil=true seems to make the build pass. So it must have something to do with the various HAVE_ABSEIL conditionals in

being off?

@keith
Copy link
Contributor

keith commented Mar 21, 2024

I think that must be because this target:

"@com_google_absl//absl/strings",
always includes absl? so if these aliases
namespace absl {
OTABSL_NAMESPACE_BEGIN
using std::bad_variant_access;
using std::get;
using std::get_if;
using std::holds_alternative;
are loaded in that dependency tree, there will be conflicts? It looks to me like abseil also unconditionally comes through the grpc dependency, so even if that direct dep is removed the same error occurs.

That doesn't explain why the update triggered it. Best I can assume is something in the dep tree changed to include the conflicting symbols? Doesn't explain why cmake doesn't have the same issue (maybe that difference isn't surprising)

@keith
Copy link
Contributor

keith commented Mar 21, 2024

Might be a crazy suggestion but would removing that absl condition be an option? to always enable it, since it's always coming from grpc anyways

@marcalff
Copy link
Member Author

Interesting.

opentelemetry-cpp in general may or may not use abseil (HAVE_ABSEIL), both builds are supported.

Now, the OTLP exporter does have a hard dependency on protobuf, which now requires to use abseil.

So, when building with the OTLP exporter, the build should use abseil.

There is also the issue, undecided at this point, if the opentelemetry-cpp build should use a OPENTELEMETRY_HAVE_ABSEIL to avoid name collision with HAVE_ABSEIL that can be set elsewhere (like by protobuf).

@marcalff
Copy link
Member Author

CMake contains the following logic:

if(WITH_OTLP_GRPC OR WITH_OTLP_HTTP)
  find_package(Protobuf)
  if(Protobuf_VERSION AND Protobuf_VERSION VERSION_GREATER_EQUAL "3.22.0")
    if(NOT WITH_ABSEIL)
      message(
        FATAL_ERROR
          "Protobuf 3.22 or upper require abseil-cpp(Recommended version: 20230125 or upper)"
      )
    endif()
  endif()

Not sure what needs to be done in bazel for the same, so that the otlp exporter is always build with abseil.

@keith
Copy link
Contributor

keith commented Mar 21, 2024

I think in bazel that condition doesn't currently make sense to exist since grpc / protobuf are never optional. which makes it sound to me like absl should just be non-optional in bazel too if so?

@marcalff marcalff added the pr:do-not-merge This PR is not ready to be merged. label Mar 21, 2024
@keith
Copy link
Contributor

keith commented Mar 21, 2024

I don't really suggest this option, but here's something that does work:

diff --git a/api/BUILD b/api/BUILD
index 0ae0061f..d196c992 100644
--- a/api/BUILD
+++ b/api/BUILD
@@ -25,34 +25,46 @@ string_flag(
     values = CPP_STDLIBS,
 )
 
+_DEFINES_SELECT = select({
+    ":set_cxx_stdlib_none": [],
+    ### automatic selection
+    ":set_cxx_stdlib_best": ["OPENTELEMETRY_STL_VERSION=(__cplusplus/100)"],
+    # See https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus
+    ":set_cxx_stdlib_best_and_msvc": ["OPENTELEMETRY_STL_VERSION=(_MSVC_LANG/100)"],
+    ### manual selection
+    ":set_cxx_stdlib_2014": ["OPENTELEMETRY_STL_VERSION=2014"],
+    ":set_cxx_stdlib_2017": ["OPENTELEMETRY_STL_VERSION=2017"],
+    ":set_cxx_stdlib_2020": ["OPENTELEMETRY_STL_VERSION=2020"],
+    ":set_cxx_stdlib_2023": ["OPENTELEMETRY_STL_VERSION=2023"],
+    "//conditions:default": [],
+})
+
 cc_library(
-    name = "api",
+    name = "api_with_abseil",
     hdrs = glob(["include/**/*.h"]),
-    defines = select({
-        ":with_external_abseil": ["OPENTELEMETRY_HAVE_ABSEIL"],
-        "//conditions:default": [],
-    }) + select({
-        ":set_cxx_stdlib_none": [],
-        ### automatic selection
-        ":set_cxx_stdlib_best": ["OPENTELEMETRY_STL_VERSION=(__cplusplus/100)"],
-        # See https://learn.microsoft.com/en-us/cpp/build/reference/zc-cplusplus
-        ":set_cxx_stdlib_best_and_msvc": ["OPENTELEMETRY_STL_VERSION=(_MSVC_LANG/100)"],
-        ### manual selection
-        ":set_cxx_stdlib_2014": ["OPENTELEMETRY_STL_VERSION=2014"],
-        ":set_cxx_stdlib_2017": ["OPENTELEMETRY_STL_VERSION=2017"],
-        ":set_cxx_stdlib_2020": ["OPENTELEMETRY_STL_VERSION=2020"],
-        ":set_cxx_stdlib_2023": ["OPENTELEMETRY_STL_VERSION=2023"],
-        "//conditions:default": [],
-    }),
+    defines = ["OPENTELEMETRY_HAVE_ABSEIL"] + _DEFINES_SELECT,
     strip_include_prefix = "include",
     tags = ["api"],
-    deps = select({
-        ":with_external_abseil": [
-            "@com_google_absl//absl/base",
-            "@com_google_absl//absl/strings",
-            "@com_google_absl//absl/types:variant",
-        ],
-        "//conditions:default": [],
+    deps = [
+        "@com_google_absl//absl/base",
+        "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/types:variant",
+    ],
+)
+
+cc_library(
+    name = "api_without_abseil",
+    hdrs = glob(["include/**/*.h"]),
+    defines = _DEFINES_SELECT,
+    strip_include_prefix = "include",
+    tags = ["api"],
+)
+
+alias(
+    name = "api",
+    actual = select({
+        ":with_external_abseil": ":api_with_abseil",
+        "//conditions:default": ":api_without_abseil",
     }),
 )
 
diff --git a/examples/grpc/BUILD b/examples/grpc/BUILD
index b1d04e9f..e45c5458 100644
--- a/examples/grpc/BUILD
+++ b/examples/grpc/BUILD
@@ -43,7 +43,7 @@ cc_binary(
     deps = [
         "messages_cc_grpc",
         ":tracer_common",
-        "//api",
+        "//api:api_with_abseil",
         "//sdk/src/trace",
         "@com_github_grpc_grpc//:grpc++",
     ],
@@ -59,7 +59,7 @@ cc_binary(
     deps = [
         "messages_cc_grpc",
         ":tracer_common",
-        "//api",
+        "//api:api_with_abseil",
         "//sdk/src/trace",
         "@com_github_grpc_grpc//:grpc++",
     ],
diff --git a/exporters/otlp/BUILD b/exporters/otlp/BUILD
index a9f1c887..b45e9d73 100644
--- a/exporters/otlp/BUILD
+++ b/exporters/otlp/BUILD
@@ -1,10 +1,10 @@
 # Copyright The OpenTelemetry Authors
 # SPDX-License-Identifier: Apache-2.0
 
-package(default_visibility = ["//visibility:public"])
-
 load("//bazel:otel_cc_benchmark.bzl", "otel_cc_benchmark")
 
+package(default_visibility = ["//visibility:public"])
+
 cc_library(
     name = "otlp_recordable",
     srcs = [
@@ -58,6 +58,7 @@ cc_library(
         "otlp_grpc",
     ],
     deps = [
+        "//api:api_with_abseil",
         "//ext:headers",
         "//sdk/src/common:global_log_handler",
         "@com_github_grpc_grpc//:grpc++",
@@ -131,7 +132,7 @@ cc_library(
         "otlp_http_log",
     ],
     deps = [
-        "//api",
+        "//api:api_with_abseil",
         "//ext/src/http/client/curl:http_client_curl",
         "//sdk:headers",
         "//sdk/src/common:base64",

It works by splitting the api cc_library into 2, instead of splitting attributes on that library based on the abseil condition, then libraries that "know" they will have abseil via grpc use the //api:api_with_abseil library instead. I think the potential downside is that both api_with_abseil and api_without_abseil could end up in the dependency tree of a single build, which sounds like it could lead to confusing issues.

@marcalff marcalff requested a review from ThomsonTan March 26, 2024 22:55
@marcalff marcalff requested a review from lalitb March 26, 2024 22:55
@marcalff
Copy link
Member Author

@lalitb @ThomsonTan @keith Please take another look.

I ended up removing the api:with_abseil option entirely from bazel, given that it is not optional.

If we take this path, I will also add notes about breaking changes in the changelog.

@marcalff
Copy link
Member Author

Any idea about the link failures on bazel windows ?

@lalitb
Copy link
Member

lalitb commented Mar 26, 2024

I ended up removing the api:with_abseil option entirely from bazel, given that it is not optional.

Would this mean that with bazel build, there won't be possibility of building with internal abseil snapshot, which is meant to be ABI safe ?

@keith
Copy link
Contributor

keith commented Mar 26, 2024

That sounds good to me! But I don't know what the use case was where folks were opting out of it with bazel either. I don't recognize the windows link error. Might not be new if that configuration wasn't tested on CI before. I don't have a windows machine to debug. I wonder if there's a related abseil issue?

@marcalff marcalff changed the title Upgrade to abseil 20240116.1 Upgrade to abseil 20240116.1 (CMake only) Mar 28, 2024
@marcalff marcalff changed the title Upgrade to abseil 20240116.1 (CMake only) [CI] Upgrade to abseil 20240116.1 (CMake only) Mar 28, 2024
@marcalff
Copy link
Member Author

I ended up removing the api:with_abseil option entirely from bazel, given that it is not optional.

Would this mean that with bazel build, there won't be possibility of building with internal abseil snapshot, which is meant to be ABI safe ?

Correct, although the concept of ABI itself does not really apply to bazel, which never installs a component somewhere, and never take dependencies from installed locations. Bazel builds tends to compile the entire dependency tree from source, making ABI compatibility a moot point.

I am abandoning this path anyway, because bazel still fails on some platforms (windows), even after a simplification.

I suspect there are many independent issues in the bazel build that accumulated other time, making it impossible hard to maintain, as each failure needs to be analyzed in depth and resolved.

@marcalff
Copy link
Member Author

@lalitb @ThomsonTan @esigo Please take yet another look.

This change affects CMake only, the bazel build needs to be fixed independently with #2619.

Given how we have a bring your own dependency model, the discrepancy in CI between CMake and bazel looks acceptable to me.

@marcalff marcalff requested a review from ThomsonTan March 28, 2024 20:45
@marcalff marcalff removed the pr:do-not-merge This PR is not ready to be merged. label Mar 28, 2024
Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, as we have separate tracking issue for bazel build. Thanks.

@marcalff marcalff merged commit fabd8cc into open-telemetry:main Mar 28, 2024
48 checks passed
@marcalff marcalff deleted the fix_upgrade_abseil branch June 3, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants